Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linting: Update linter to enforce ES 2018 features #25470

Merged
merged 4 commits into from
Feb 15, 2023
Merged

Linting: Update linter to enforce ES 2018 features #25470

merged 4 commits into from
Feb 15, 2023

Conversation

epreston
Copy link
Contributor

@epreston epreston commented Feb 9, 2023

Fixed #25185.

Description

Update linter to enforce ES 2018 features.

This has four changes:

  • add setting to configure linter to enforce 2018 features
  • separate config for linting into config file.
  • update the 3 header imports that were using 2020 features
  • to maintain the same public interface, add export objects MathUtils, AnimationUtils, and DataUtils

The setting that does the work of enforcing 2018 features is:

  "parserOptions": {
    "ecmaVersion": 2018,
    "sourceType": "module"
  },

The 'module' setting has been provided by the custom style plugin, it is duplicated here for clarity. This also ensures that the style plugin is only providing styles rules.

This also overrides the environment setting from the custom plugin to ensure only 2018 globals are defined. Browser and node settings are duplicated for clarity. This also ensures that the style plugin is only providing styles rules.

  "env": {
    "browser": true,
    "node": true,
    "es2018": true
  },

The lines in 'Three.js' using 2020 syntax were:

export * as AnimationUtils from './animation/AnimationUtils.js';
export * as MathUtils from './math/MathUtils.js';
export * as DataUtils from './extras/DataUtils.js';

With an export object added, these become:

export { AnimationUtils } from './animation/AnimationUtils.js';
export { MathUtils } from './math/MathUtils.js';
export { DataUtils } from './extras/DataUtils.js';

If the project wants to remove a dependency, we can copy the 15 lines of style settings from the plugin into the lint config file.

move eslintConfig to its own file to keep package.json clean
modifications and commits not bound to or carry the risk of dependency changes.
This commit is the equivalent configuration as the starting point.
Convert ES 2020 imports into 2018.
Set project to ES 2018
@epreston epreston changed the title Linting: Update linter to enforce ES features Linting: Update linter to enforce ES 2018 features Feb 9, 2023
@epreston
Copy link
Contributor Author

epreston commented Feb 9, 2023

MathUtils, AnimationUtils, and DataUtils need to be updated to export their namespaces.

@epreston epreston marked this pull request as draft February 9, 2023 03:00
MathUtils, AnimationUtils, and DataUtils need to be updated to export their namespaces.  Privately, individual functions are exported to support the other parts of the library, publicly they are namespaced with an object wrapper.
@epreston epreston marked this pull request as ready for review February 9, 2023 03:29
@epreston
Copy link
Contributor Author

epreston commented Feb 9, 2023

If the project wants to remove a dependency, copy the 15 lines of style settings into the lint config file.

@epreston
Copy link
Contributor Author

epreston commented Feb 9, 2023

Anyone worried about tree-shaking:

As a confirmation take a look at what we ship now using the 2020 syntax. I'm going to use the middle size one as an example to reduce the reading.

The old 2020 line:

export * as AnimationUtils from './animation/AnimationUtils.js';

Is currently being translated to:

var AnimationUtils = /*#__PURE__*/Object.freeze({
	__proto__: null,
	arraySlice: arraySlice,
	convertArray: convertArray,
	flattenJSON: flattenJSON,
	getKeyframeOrder: getKeyframeOrder,
	isTypedArray: isTypedArray,
	makeClipAdditive: makeClipAdditive,
	sortedArray: sortedArray,
	subclip: subclip
});

This has been updated to:

const AnimationUtils = {
	arraySlice: arraySlice,
	convertArray: convertArray,
	isTypedArray: isTypedArray,
	getKeyframeOrder: getKeyframeOrder,
	sortedArray: sortedArray,
	flattenJSON: flattenJSON,
	subclip: subclip,
	makeClipAdditive: makeClipAdditive
};

It gets translated to:

const AnimationUtils = {
	arraySlice: arraySlice,
	convertArray: convertArray,
	isTypedArray: isTypedArray,
	getKeyframeOrder: getKeyframeOrder,
	sortedArray: sortedArray,
	flattenJSON: flattenJSON,
	subclip: subclip,
	makeClipAdditive: makeClipAdditive
};

Keeping it clean, the final bundler (client application) works its magics. Its possible we get slightly better tree-shake using the 2018 syntax. Keep in mind this is an isolated context, relating to a library (not client application), and a very specific import command.

In the old code, final bundler can only remove the whole object if its entirely unused, which will be created in any case (new or old code). Tools can remove constants if they are entirely unused, so we have even ground.

@epreston
Copy link
Contributor Author

epreston commented Feb 9, 2023

For evaluation, a test rig of the different builds and tree shake effects can be found here:

https://github.com/epreston/three-treeshake-coverage

Comment and uncomment the different imports, build and inspect the results in the dist folder.

In all cases, AnimationUtils is correctly tree-shaken.

@donmccurdy
Copy link
Collaborator

Is it possible to configure the linter to enforce ES2018 features, but to ignore ES2020 import/export syntax? I don't think there is any problem with shipping those ES2020 exports, and might allow us to make a narrower change here.

If that's not possible, that's OK, but I didn't see that mentioned in the thread so far.

@epreston
Copy link
Contributor Author

epreston commented Feb 9, 2023

Anything's possible with configuration and tooling. We have great ideas for partial solutions going 2020 with restrictions. I don't see a built in way to approach the problem from that direction (2018 with 2020 imports).

In all cases, we never ship these exports. The code this replaces is a developer convenience to shave 20 lines off the code we maintain. The code it generates is equal. What we trade is complexity, validation of PRs, and standards with partial solutions.

@donmccurdy
Copy link
Collaborator

I see – thanks! No concerns about the exports in that case, changing them to reflect how they already get shipped in the bundle sounds good.

@marcofugaro
Copy link
Contributor

Yes this PR does not affect tree-shaking, nor does it make it better.

My point is that this PR changes the exports in a confusing way for no apparent reason, since the import/exports get compiled away. I don't see how this change is beneficial for future maintainers to understand what is going on. The real issue is with eslint, a tool shouldn't dictate how we structure our code.

But I guess this can be overlooked one since it's supposed to be temporary.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Feb 9, 2023

We want to have automated warnings when our source code is not ES2018 compliant. I would've ideally excluded imports/exports from that, since they're handled by the build process anyway, but that path doesn't seem to be practical, so I'm fine with changing the imports/exports this way for now. We can clean them up later if needed.

It's not just about 1-2 unsupported ES2020 features — we want our code to be ES2018 compliant, and we don't want to be surprised by a bit of newer syntax sneaking in, or surprised by transpilers doing something we didn't expect. I worry that just enforcing ES2020 and adding custom rules on top will not give us those guarantees, is that wrong?

@epreston
Copy link
Contributor Author

epreston commented Feb 9, 2023

I'm confused. Changing the only 3 imports that are using a different syntax so they are the same as other 100 adds confusion ? The lines that created confusion about tree-shaking ?

The project agrees on a syntax level, they is set in one line, PR's get validated against it, no gaps. You set any tool to 2018 and you get the same results. Seems kinda clean having it configured according to the manual in the most typical way.

@epreston
Copy link
Contributor Author

epreston commented Feb 9, 2023

we don't want to be surprised by a bit of newer syntax sneaking in

Please don't look at line 1891 of examples/GLTFExporter.js until I submit a PR.

@marcofugaro
Copy link
Contributor

Hmm yeah I guess there is no way of having full ES2018 coverage without changing the import as well.

@mrdoob
Copy link
Owner

mrdoob commented Feb 10, 2023

Ready to merge? 🧐

@marcofugaro
Copy link
Contributor

Go ahead!

@Mugen87 Mugen87 added this to the r150 milestone Feb 10, 2023
@epreston
Copy link
Contributor Author

@mrdoob can we wrap this up ? I'd like to move on to the next.

@mrdoob mrdoob merged commit b2fdb55 into mrdoob:dev Feb 15, 2023
@epreston epreston deleted the linting-update-config branch February 15, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update linter to enforce ES features
7 participants